Remove main thread block on allocation #216
Conversation
remote allocation from main requestmanager thread -- this could block processing of other operations like request cancels
| rm.processTerminations(filteredResponses) | ||
| select { | ||
| case <-rm.ctx.Done(): | ||
| case prm.response <- nil: |
There was a problem hiding this comment.
Why isn't sending nil still needed? Or was it not needed previously?
There was a problem hiding this comment.
prm.response as a field was removed in this PR I think. Waiting on the response channel was the essentially what was used to block on allocation, but that's now just being done by allocating directly in the receive handler.
| @@ -31,7 +31,6 @@ type alternateQueue struct { | |||
|
|
|||
| // Allocator indicates a mechanism for tracking memory used by a given peer | |||
| type Allocator interface { | |||
| AllocateBlockMemory(p peer.ID, amount uint64) <-chan error | |||
| ReleaseBlockMemory(p peer.ID, amount uint64) error | |||
There was a problem hiding this comment.
So now an Allocator interface only has a ReleaseBlockMemory method. Is Allocator still an appropriate name?
There was a problem hiding this comment.
This is just that go pattern of defining the interface at the site of usage. I guess we could rename it? Not sure.
| totalMemoryAllocated += uint64(len(blk.RawData())) | ||
| } | ||
| select { | ||
| case <-gsr.graphSync().responseAllocator.AllocateBlockMemory(sender, totalMemoryAllocated): |
There was a problem hiding this comment.
If all the message receivers use the same allocator, would this still cause all other receivers to be blocked on memory allocation?
There was a problem hiding this comment.
The allocator takes a peer as a parameter cause it tracks memory allocations per peer as well as total. There's a total limit and a per peer limit. So if peer X tries to allocate above a certain amount, it will get held up seperately long before the total limit across peers gets hit.
Goals
When memory backpressure for requests was introduced, it was introduced in such a way that not only blocks processing of further libp2p messages, but also blocks the main thread of the request manager. This may have serious side effects, as this effectively can cause the request manager to sieze up and not processing other commands like cancelling requests.
Implementation
Move potentially blocking memory allocation out of the main AsyncLoader & the internals of the main RequestManager thread, and into the per-peer thread of the libp2p message handler. This has the same effect of blocking reading further libp2p messages on the stream, and preserving memory, while at the same time not blocking the request manager thread.
Also removes parameter from AsyncLoader.ProcessResponses that is now spurious.